Do not immediately fail if git is not available #1074

astrojuanlu · 2023-04-02T21:11:06Z

There are several ways of doing this, I picked one that I considered simple enough. The idea is to always import git from a centralized place, and in that place do a soft fail if git fails to be imported from plumbum.cmd.

This is how copier --version looks like with these changes:

root@885f691c5e79:/# which git || echo "git is not available"
git is not available
root@885f691c5e79:/# copier --version
/usr/local/lib/python3.10/site-packages/copier/vcs.py:25: UserWarning: git command not found, some functionality might not work
  warnings.warn(
copier 7.1.0a0.post98.dev0+5d600ff

On the other hand, I made copier.vcs.is_git_bundle more consistent with is_git_shallow_repo and is_in_git_repo, and now it returns False if there is an OSError or ProcessExecutionError error. Thanks to that, now copier can work with gitless templates as well, as mentioned in #1045 (comment):

root@885f691c5e79:/# copier /home/copier-pylib-main/ /tmp/
/usr/local/lib/python3.10/site-packages/copier/vcs.py:25: UserWarning: git command not found, some functionality might not work
  warnings.warn(
🎤 Your account or organization
   astrojuanlu
...

Copying from template version None
 identical  .
    create  .gitignore
...

I have two questions:

First, I have no idea how to test this. Initially, I tried some form of mocking or monkeypatching, but since plumbum.cmd.git is not an actual attribute, things are more complicated:

    @pytest.fixture
    def no_plumbum_git(monkeypatch):
>       monkeypatch.delattr("plumbum.cmd.git")
E       AttributeError: git

tests/test_main.py:6: AttributeError

I could do a very deep patching of how plumbum.local retrieves the command, but wanted to ask for advice first.

As I explained in the second commit, one could actually consider that copier.vcs.is_git_bundle, is_git_shallow_repo, and is_in_git_repo should return None or something else than True or False, if the git status could not be determined because the git command is missing. In this scheme, returning None would mean "I don't know if this directory is actually a git repo". For this reason, I preferred to wait before proceeding any further adding tests.

Let me know what you folks think.

For the record, I too was a bit puzzled by the heavyweight requirements to run the linter #931 so going forward I think I'll use Gitpod to continue this PR (I have only used Nix a bit when experimenting with Replit earlier this year, not sure if I already want to commit to installing it on my home laptop).

yajo

About testing, you can obtain the git interpreter through plumbum.local["git"] instead. That'll be easier to mock.

https://plumbum.readthedocs.io/en/latest/local_commands.html#guide-local-commands

copier/vcs.py

astrojuanlu · 2023-08-06T15:08:42Z

This is ready for a second pass. I think now it will be easier to test (haven't started doing it yet).

On the other hand, my other question still stands:

As I explained in the second commit, one could actually consider that copier.vcs.is_git_bundle, is_git_shallow_repo, and is_in_git_repo should return None or something else than True or False, if the git status could not be determined because the git command is missing. In this scheme, returning None would mean "I don't know if this directory is actually a git repo". For this reason, I preferred to wait before proceeding any further adding tests.

yajo · 2023-08-24T12:48:54Z

As I explained in the second commit, one could actually consider that copier.vcs.is_git_bundle, is_git_shallow_repo, and is_in_git_repo should return None or something else than True or False, if the git status could not be determined because the git command is missing. In this scheme, returning None would mean "I don't know if this directory is actually a git repo". For this reason, I preferred to wait before proceeding any further adding tests.

I don't think this is a good solution.

Instead, those functions should be able to return a boolean without relying in the presence of git. That shouldn't be too hard (just check if there's a .git file/dir in the folder or any of its parents, usually).

This way, if it really is a git repo and git is not available, then copier should fail.

copier/vcs.py

codecov · 2023-08-24T12:53:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.08%. Comparing base (00e51bd) to head (d1b0b45).
Report is 309 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1074   +/-   ##
=======================================
  Coverage   97.07%   97.08%           
=======================================
  Files          48       48           
  Lines        4143     4149    +6     
=======================================
+ Hits         4022     4028    +6     
  Misses        121      121

Flag	Coverage Δ
unittests	`97.08% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Fix copier-orggh-312.

astrojuanlu · 2023-08-26T14:27:22Z

Applied changes and ran poe lint (doing so from Gitpod was more or less straightforward 👍🏽)

astrojuanlu · 2023-08-26T14:28:25Z

As I explained in the second commit, one could actually consider that copier.vcs.is_git_bundle, is_git_shallow_repo, and is_in_git_repo should return None or something else than True or False, if the git status could not be determined because the git command is missing. In this scheme, returning None would mean "I don't know if this directory is actually a git repo". For this reason, I preferred to wait before proceeding any further adding tests.

I don't think this is a good solution.

Instead, those functions should be able to return a boolean without relying in the presence of git. That shouldn't be too hard (just check if there's a .git file/dir in the folder or any of its parents, usually).

This way, if it really is a git repo and git is not available, then copier should fail.

I'm reverting the corresponding commit for now

sharpencrag · 2023-08-28T20:30:46Z

Apologies if this is not the right place to bring it up, but after this merge, I am still seeing a failure when attempting to run copier without git installed.

is_git_bundle is still calling get_git without handling the exception when it fails

astrojuanlu · 2023-08-28T23:24:13Z

See #1074 (comment), left it out of this PR

astrojuanlu mentioned this pull request Apr 2, 2023

Allow to execute when git is not installed #312

Closed

yajo requested changes Apr 7, 2023

View reviewed changes

copier/vcs.py Outdated Show resolved Hide resolved

yajo marked this pull request as draft April 17, 2023 11:57

astrojuanlu force-pushed the fix-no-git branch from d5b6acf to 064c2be Compare August 6, 2023 15:06

astrojuanlu marked this pull request as ready for review August 6, 2023 15:08

astrojuanlu force-pushed the fix-no-git branch from 064c2be to 4e0360a Compare August 6, 2023 15:13

astrojuanlu mentioned this pull request Aug 6, 2023

Investigate how we can remove Kedro's dependency on git (for starters) kedro-org/kedro#2051

Closed

yajo requested changes Aug 24, 2023

View reviewed changes

copier/vcs.py Outdated Show resolved Hide resolved

copier/vcs.py Outdated Show resolved Hide resolved

copier/vcs.py Outdated Show resolved Hide resolved

astrojuanlu force-pushed the fix-no-git branch 2 times, most recently from 63c635f to 0339e7c Compare August 26, 2023 13:50

feat: Do not immediately fail if git is not available

aba7e1d

Fix copier-orggh-312.

astrojuanlu force-pushed the fix-no-git branch from 0339e7c to b5130c7 Compare August 26, 2023 14:23

astrojuanlu added 4 commits August 26, 2023 14:38

feat: Improve lazy loading of git

2c76940

feat: Simplify git detection

85b0c50

feat: Replace GIT_VERSION constant with lazy function

8821588

style: Lint

d1b0b45

astrojuanlu force-pushed the fix-no-git branch from b5130c7 to d1b0b45 Compare August 26, 2023 14:38

yajo approved these changes Aug 28, 2023

View reviewed changes

yajo enabled auto-merge (squash) August 28, 2023 16:44

yajo merged commit 9406716 into copier-org:master Aug 28, 2023

astrojuanlu deleted the fix-no-git branch August 28, 2023 17:33

astrojuanlu mentioned this pull request Aug 28, 2023

Replace git subprocess by pygit2 #1045

Open

yajo mentioned this pull request Aug 29, 2023

Failure when attempting to run copier without git installed. #1304

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not immediately fail if git is not available #1074

Do not immediately fail if git is not available #1074

astrojuanlu commented Apr 2, 2023

yajo left a comment •

edited

Loading

astrojuanlu commented Aug 6, 2023 •

edited

Loading

yajo commented Aug 24, 2023

codecov bot commented Aug 24, 2023 •

edited

Loading

astrojuanlu commented Aug 26, 2023

astrojuanlu commented Aug 26, 2023 •

edited

Loading

sharpencrag commented Aug 28, 2023

astrojuanlu commented Aug 28, 2023

Do not immediately fail if git is not available #1074

Do not immediately fail if git is not available #1074

Conversation

astrojuanlu commented Apr 2, 2023

yajo left a comment • edited Loading

Choose a reason for hiding this comment

astrojuanlu commented Aug 6, 2023 • edited Loading

yajo commented Aug 24, 2023

codecov bot commented Aug 24, 2023 • edited Loading

Codecov Report

astrojuanlu commented Aug 26, 2023

astrojuanlu commented Aug 26, 2023 • edited Loading

sharpencrag commented Aug 28, 2023

astrojuanlu commented Aug 28, 2023

yajo left a comment •

edited

Loading

astrojuanlu commented Aug 6, 2023 •

edited

Loading

codecov bot commented Aug 24, 2023 •

edited

Loading

astrojuanlu commented Aug 26, 2023 •

edited

Loading